Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fpm deployment: workaround for case-insensitive fs #790

Merged
merged 1 commit into from
Apr 10, 2024

Conversation

perazz
Copy link
Contributor

@perazz perazz commented Apr 9, 2024

cf. #787 (comment)

I understand the CI script is not supposed to be OS-agnostic, it should only live within the repo.
However, it seems like people use the stdlib-fpm branch more than just as an fpm dependency, so, this fix is probably useful.

cc: @jalvesz @jvdp1

@jalvesz
Copy link
Contributor

jalvesz commented Apr 9, 2024

Thanks @perazz, the work around works indeed! I'm just thinking if we should take the time to propose a stable solution?
I understand what you mean here:

I understand the CI script is not supposed to be OS-agnostic, it should only live within the repo.

But then, if when developing one already sees that "people is not supposed to, but could be able to" and that the solution is available and also proposes less friction than the alternative which is supposed to be the "standard way" people will use the non-standard.

On my side I would like to keep on working on the fypp files (as I'm trying to get more experience with it) and be sure that the whole process is stable (from developing the .fypp to seeing the prompt of the test after running), but I would like to have the less friction between modifying files and running, which with fpm I get just that! If I can avoid switching branches with git just for building, I'll preferer to avoid it. Just some food-for-thought

My current workflow for working in stdlib (just for making things clear): I do not use the stdlib-fpm git branch per-se, I take the main branch, that I locally build by applying in a single line:

source ./ci/fpm-deployment.sh; cd stdlib-fpm/; fpm build --profile release

So then, in a single line, I can very quickly check that it builds when modifying the .fypp files.

@perazz
Copy link
Contributor Author

perazz commented Apr 9, 2024

@jalvesz This is part of a larger issue/concern that affects the double build system (CMake vs. stdlib-fpm): they currently do not offer the same options. For example:

  • support for xdp/qp kinds: it is turned off in the fpm branch.
  • (future) selection of external BLAS/LAPACK backends will have to be figured out.

Some of it could be solved by implementing more features in fpm (fypp preprocessing, package features). But these are long-term goals. For now I'd suggest to just fix this patch, so that the script can run in WSL and then if you want to contribute an update to the script as Python, it'd be great! a python environment is always going to be available, because fypp also needs it.

@jvdp1
Copy link
Member

jvdp1 commented Apr 9, 2024

I am fine with these changes.
However, I agree that it becomes quite an issue. We should really put some effort to fully support fypp in fpm. This support would remove the need of the branch fpm-stdlib (and all these issues).

@jalvesz : here is how I build the master branch of stdlib

fpm build --compiler fypp_ifort.py

(just replace ifort by gfortran or any other compiler, if needed)

The fpm.toml file is:

name = "stdlib"
version = "VERSION"
license = "MIT"
author = "stdlib contributors"
maintainer = "@fortran-lang/stdlib"
copyright = "2019-2021 stdlib contributors"

[dev-dependencies]
test-drive.git = "https://github.com/fortran-lang/test-drive"
test-drive.tag = "v0.4.0"

[preprocess]
[preprocess.cpp]
suffixes = [".F90", ".f90", ".fypp"]
macros = [
  "MAXRANK=3",
  "PROJECT_VERSION_MAJOR=0",
  "PROJECT_VERSION_MINOR=0",
  "PROJECT_VERSION_PATCH=0"
]

The fypp_ifort.py file is:

#!/usr/bin/env python3
import sys
import subprocess
from pathlib import Path

# Read the system arguments.
args = sys.argv[1:]

# Get the filenames with .fypp extension and convert to string.
source_file = [arg for arg in args if arg.endswith(".fypp")]
output_file = [arg for arg in args if arg.endswith(".o")]

if len(source_file) != len(output_file):
    subprocess.run(["ifort"] + args, check=True)
    sys.exit(0)

source_file = source_file[0]
output_file = output_file[0]
preprocessed = output_file.replace(".o", ".f90")

# Filter out the macro definitions.
macros = [arg for arg in args if arg.startswith("-D")]

# Filter out the include paths with -I prefix.
include_paths = [arg for arg in args if arg.startswith("-I")]

subprocess.run(
    ["fypp", "-n", source_file, preprocessed] + macros + include_paths,
    check=True
)

args = [arg for arg in args if arg != source_file and not arg in macros] + [preprocessed]
subprocess.run(["ifort"] + args, check=True)

This approach is based on fortran-lang/fpm#729

However, I am doubting to advertise this approach. Or we should provide the Python script for all compilers?

@perazz
Copy link
Contributor Author

perazz commented Apr 9, 2024

I like this idea @jvdp1. I noticed that the stdlib-fpm branch is seen by some users as an "easy" way to just grab the Fortran code without having to go through CMake (for developing, I use CMake and yes, it is very slow/cumbersome). So, supporting fypp in fpm is definitely necessary at this point, although it goes in the other direction (source files would be un-processed .fypp files). I just want to say that I think that either way, keeping a branch with "clean" Fortran sources only is probably going to be always needed

@jalvesz
Copy link
Contributor

jalvesz commented Apr 9, 2024

@jvdp1 that's a very nice approach, kind of like what I ended up doing here https://github.com/jalvesz/FSPARSE/blob/main/deployement.py but better :)

I also use CMake with my local main build system, so I'm trying to get a workflow that is as close as possible regardless of using fpm or CMake. Right now, my solution is that I do not define a separate branch for storing *.fypp and .f90 files, both reside in the same repository, it might look redundant, but given that .fypp files cannot be debugged, it is the only way to easily edit the macro files (fypp) and then debug the actual sources. With CMake, I defined a PRE_BUILD action with add_custom_command which enables me to even call my python preprocessing script with Microsoft Visual Studio while developing in windows. I'll see live the .f90 being recreated as I work on the .fypp's

I could try to propose a python script to improve the fpm workflow locally for stdlib in the mean-time a more robust solution is found directly in fpm following what @jvdp1 showed ( ? )

In any case, I'm ok with merging this PR to solve the WSL problem.

Copy link
Member

@jvdp1 jvdp1 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am ok with the changes. Thank you @perazz

@jvdp1
Copy link
Member

jvdp1 commented Apr 9, 2024

I just want to say that I think that either way, keeping a branch with "clean" Fortran sources only is probably going to be always needed

Indeed. We will need to agree which features are the default ones.

@jvdp1
Copy link
Member

jvdp1 commented Apr 9, 2024

... but given that .fypp files cannot be debugged, it is the only way to easily edit the macro files (fypp) and then debug the actual sources.

With fypp, I use the following option:

  -n, --line-numbering  emit line numbering markers

Then with ifort, warning and error messages relate to the line position in the fypp files, and not in the generated f90 files. This helps me to debug with fypp files, instead of 90 files.

I could try to propose a python script to improve the fpm workflow locally for stdlib in the mean-time a more robust solution is found directly in fpm following what @jvdp1 showed ( ? )

I think it would be welcomed and appreciated.

In any case, I'm ok with merging this PR to solve the WSL problem.

I agree too.

@jalvesz jalvesz mentioned this pull request Apr 9, 2024
@perazz
Copy link
Contributor Author

perazz commented Apr 10, 2024

While the discussion of #791 develops, I will merge this one, thanks @jvdp1 @jalvesz for the reviews.

@perazz perazz merged commit e19d4b6 into fortran-lang:master Apr 10, 2024
17 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants